Add Day_Of_Week Function As An Alias Of DayOfWeek by GabeFernandez310 · Pull Request #190 · Bit-Quill/opensearch-project-sql

GabeFernandez310 · 2022-12-15T23:11:38Z

Description

Adds support for the day_of_week function as an alias for the dayofweek function which currently exists in opensearch

Issues Resolved

#722

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

codecov · 2022-12-15T23:19:12Z

Codecov Report

Merging #190 (20150cf) into integ-add-day_of_week-function (c923e80) will decrease coverage by 2.49%.
The diff coverage is 100.00%.

@@                         Coverage Diff                          @@
##             integ-add-day_of_week-function     #190      +/-   ##
====================================================================
- Coverage                             98.31%   95.81%   -2.50%     
- Complexity                             3521     3524       +3     
====================================================================
  Files                                   342      352      +10     
  Lines                                  8700     9365     +665     
  Branches                                554      673     +119     
====================================================================
+ Hits                                   8553     8973     +420     
- Misses                                  142      334     +192     
- Partials                                  5       58      +53

Flag	Coverage Δ
query-workbench	`62.76% <ø> (?)`
sql-engine	`98.31% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...c/main/java/org/opensearch/sql/expression/DSL.java	`100.00% <100.00%> (ø)`
...arch/sql/expression/datetime/DateTimeFunction.java	`100.00% <100.00%> (ø)`
...h/sql/expression/function/BuiltinFunctionName.java	`100.00% <100.00%> (ø)`
workbench/public/application.tsx	`0.00% <0.00%> (ø)`
workbench/public/components/Header/Header.tsx	`100.00% <0.00%> (ø)`
workbench/public/components/app.tsx	`0.00% <0.00%> (ø)`
...h/public/components/QueryLanguageSwitch/Switch.tsx	`85.71% <0.00%> (ø)`
...ublic/components/QueryResults/QueryResultsBody.tsx	`68.32% <0.00%> (ø)`
workbench/public/utils/PanelWrapper.tsx	`100.00% <0.00%> (ø)`
...ch/public/components/QueryResults/QueryResults.tsx	`61.60% <0.00%> (ø)`
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

docs/user/dql/functions.rst

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

MitchellGale · 2022-12-19T19:52:59Z

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

+    testDayOfWeekWithUnderscoress(DSL.day_of_week(DSL.literal("2021-02-28")), 1);
+
+    //Feb. 29 of a non-leap year
+    assertThrows(SemanticCheckException.class, () ->  testInvalidDayOfWeek("2021-02-29"));


MitchellGale · 2022-12-19T19:56:24Z

docs/user/dql/functions.rst

+    | 4                                              |
+    +------------------------------------------------+
+
+    os> SELECT DAY_OF_WEEK(TIMESTAMP('2020-08-26 00:00:00'))


consider having both dayofweek and day_of_week on the same line to make comparisons easier to show equivalence.

eg. SELECT DAYOFWEEK(DATETIME('2020-08-26 00:00:00')), DAY_OF_WEEK(DATETIME('2020-08-26 00:00:00'));

also consider removing a lot of these from the documentation. It's a little cumbersome and the variants are already tested in DateTimeFunctionIT. I'd just include one example for DAYOFWEEK and one example of DAY_OF_WEEK or combine them like Mitchell suggested.

Fixed in e571bf0

acarbonetto · 2022-12-19T21:07:48Z

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

    assertEquals(integerValue(1), eval(expression));
  }

+  public void testDayOfWeekWithUnderscoress(FunctionExpression dateExpression, int dayOfWeek) {


Fixed in f044830

acarbonetto · 2022-12-19T21:07:56Z

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

+    );
+  }
+
+  public void testInvalidDayOfWeek(String date) {


Fixed in f044830

acarbonetto · 2022-12-19T21:12:52Z

docs/user/dql/functions.rst

+    | 4                                              |
+    +------------------------------------------------+
+
+    os> SELECT DAY_OF_WEEK(TIMESTAMP('2020-08-26 00:00:00'))


also consider removing a lot of these from the documentation. It's a little cumbersome and the variants are already tested in DateTimeFunctionIT. I'd just include one example for DAYOFWEEK and one example of DAY_OF_WEEK or combine them like Mitchell suggested.

acarbonetto · 2022-12-19T21:14:36Z

sql/src/main/antlr/OpenSearchSQLParser.g4

    : CURRENT_DATE
    | CURRENT_TIME
    | CURRENT_TIMESTAMP
+    | DAY_OF_WEEK


I don't understand why we don't also need DAYOFWEEK defined here.

Same as #195 (comment)

Fixed in e571bf0

Yury-Fridlyand

mysql> select dayofweek(curtime());
+----------------------+
| dayofweek(curtime()) |
+----------------------+
|                    2 |
+----------------------+
1 row in set (0.00 sec)

Function could accept TIME arg too and use FunctionProperties to resolve date from it.

Yury-Fridlyand · 2022-12-19T23:00:06Z

sql/src/main/antlr/OpenSearchSQLParser.g4

    : CURRENT_DATE
    | CURRENT_TIME
    | CURRENT_TIMESTAMP
+    | DAY_OF_WEEK


Same as #195 (comment)

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

GabeFernandez310 · 2022-12-21T22:54:46Z

mysql> select dayofweek(curtime());
+----------------------+
| dayofweek(curtime()) |
+----------------------+
|                    2 |
+----------------------+
1 row in set (0.00 sec)
Function could accept TIME arg too and use FunctionProperties to resolve date from it.

This has been implemented in 14279ed, although I am not sure if it completely follows the behaviour you were looking for. Your query of dayofweek(curtime()) or dayofweek(time('12:23:34')) now works, but a query such as dayofweek('12:23:34') does not work since the argument is of type STRING instead of the type TIME.

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

acarbonetto · 2022-12-29T21:13:27Z

docs/user/dql/functions.rst


 Usage: dayofweek(date) returns the weekday index for date (1 = Sunday, 2 = Monday, …, 7 = Saturday).

+The `day_of_week` function is also provided as an alias.


do we need an underscore to use this as an alias?

Yes you do need the underscores. Both dayofweek and day_of_week work, but the version with the underscores is an alias to dayofweek which already exists and is implemented. However, day_ofweek and dayof_week do not work if that is what you are asking.

Yury-Fridlyand · 2023-01-04T00:03:51Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

+        implWithProperties((functionProperties, arg) -> DateTimeFunction.dayOfWeekToday(
+            functionProperties.getQueryStartClock()), INTEGER, TIME),


Suggested change

implWithProperties((functionProperties, arg) -> DateTimeFunction.dayOfWeekToday(

functionProperties.getQueryStartClock()), INTEGER, TIME),

implWithProperties(nullMissingHandlingWithProperties(functionProperties, arg) -> DateTimeFunction.dayOfWeekToday(

functionProperties.getQueryStartClock())), INTEGER, TIME),

And all related changes

Works as is without needing to checkout files from other branches. Fixed in 8daa4ba

Yury-Fridlyand · 2023-01-04T00:04:37Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

   * Day of Week implementation for ExprValue.
   *
-   * @param date ExprValue of Date/String type.
+   * @param date ExprValue of Date/Datetime/String type.


and time and timestamp

Added Timestamp. Added javadoc comment to dayOfWeekToday since this is used instead for the TIME type. Done in 8daa4ba.

Yury-Fridlyand · 2023-01-04T00:36:43Z

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

-    FunctionExpression expression = DSL.dayofweek(DSL.literal(new ExprDateValue("2020-08-07")));
+    FunctionExpression expression = DSL.dayofweek(
+        functionProperties,
+        DSL.literal(new ExprDateValue("2020-08-07")));


Try @ParametrizedTest and/or split into different tests and/or use assertAll

Fixed in 20150cf

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

…project#1228) Added Implementation And Testing For Day_Of_Week Function Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com> Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

…project#1228) (opensearch-project#1239) Added Implementation And Testing For Day_Of_Week Function Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com> Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com> (cherry picked from commit bac9c37) Co-authored-by: GabeFernandez310 <gabrielf@bitquilltech.com>

GabeFernandez310 added 3 commits December 15, 2022 09:05

Added Tests

7832301

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

Added Implementation

4274273

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

Added Documentation

4a6f083

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

Fixed Checkstyle Issues

4853304

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

GabeFernandez310 marked this pull request as ready for review December 16, 2022 15:40

GabeFernandez310 requested a review from a team December 16, 2022 16:10

MitchellGale reviewed Dec 16, 2022

View reviewed changes

docs/user/dql/functions.rst Outdated Show resolved Hide resolved

Added Doctests

9a061ea

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

GabeFernandez310 requested review from a team and MitchellGale December 19, 2022 19:46

MitchellGale reviewed Dec 19, 2022

View reviewed changes

MitchellGale approved these changes Dec 19, 2022

View reviewed changes

MitchellGale requested review from GumpacG, MaxKsyunz, Yury-Fridlyand, acarbonetto, forestmvey and margarit-h December 19, 2022 19:57

acarbonetto approved these changes Dec 19, 2022

View reviewed changes

Yury-Fridlyand reviewed Dec 19, 2022

View reviewed changes

Yury-Fridlyand suggested changes Dec 19, 2022

View reviewed changes

GumpacG approved these changes Dec 20, 2022

View reviewed changes

GabeFernandez310 added 2 commits December 21, 2022 08:14

Fixed Function Names And Public Functions

f044830

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

Fixed Parser and Documentation

e571bf0

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

Yury-Fridlyand approved these changes Dec 21, 2022

View reviewed changes

Added Support For Time Type

14279ed

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

Added Basic Test For Time Type

0f08311

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

GabeFernandez310 added 2 commits December 21, 2022 16:37

Fixed Unit Test

0db6299

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

Fixed Checkstyle

2364e68

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

GabeFernandez310 requested a review from acarbonetto December 22, 2022 16:21

Altered Implementation And Test For Time Type

996d300

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

GabeFernandez310 requested a review from Yury-Fridlyand December 27, 2022 18:35

acarbonetto approved these changes Dec 29, 2022

View reviewed changes

Yury-Fridlyand reviewed Jan 4, 2023

View reviewed changes

GabeFernandez310 added 2 commits January 4, 2023 15:42

Added Support For Null And Missing Args, Fixed Comments

8daa4ba

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

Refactored Tests

20150cf

Signed-off-by: GabeFernandez310 <gabrielf@bitquilltech.com>

GabeFernandez310 merged commit cc7d920 into integ-add-day_of_week-function Jan 5, 2023


		Usage: dayofweek(date) returns the weekday index for date (1 = Sunday, 2 = Monday, …, 7 = Saturday).

		The `day_of_week` function is also provided as an alias.

		implWithProperties((functionProperties, arg) -> DateTimeFunction.dayOfWeekToday(
		functionProperties.getQueryStartClock()), INTEGER, TIME),

Conversation

GabeFernandez310 commented Dec 15, 2022

Description

Issues Resolved

Check List

Uh oh!

codecov bot commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GabeFernandez310 commented Dec 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GabeFernandez310 Dec 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Dec 15, 2022 •

edited

Loading

GabeFernandez310 Dec 29, 2022 •

edited

Loading